Fix mutable default arguments in OrderingFilter methods #9742
Conversation
There was a problem hiding this comment.
Due to how filter are used, I don't think that's ever a problem in practice (we seem to always pass the context), but shouldn't hurt to fix anyway. The only thing that could break is if None is valid value for context, and the user explicitly passes None. In which case, this would override it with an empty dict {}. Can't think of a case where that would be a problem though...
Fix looks good to me, but would be nice to add a test or 2 to make sure this doesn't regress.
Thanks
|
@browniebroke I'll look into this this week. |
|
Any updates on this? Looks like we might be able to catch this by adding |
- Fixed get_default_valid_fields() and get_valid_fields() methods in filters.py
- Changed context={} default parameter to context=None to prevent mutable default anti-pattern
- Added proper None checking with context = {} assignment inside methods
Why this fix is important:
- Mutable default arguments (context={}) create shared state across function calls
- Same dict object gets reused, potentially causing unexpected side effects
- This is a well-known Python anti-pattern that can lead to bugs
What was changed:
- Line 249: get_default_valid_fields(self, queryset, view, context=None)
- Line 285: get_valid_fields(self, queryset, view, context=None)
- Added 'if context is None: context = {}' in both methods
Testing results:
- All existing filter tests pass (pytest tests/test_filters.py)
- Custom verification script confirms fix works correctly
- Maintains backward compatibility
- No breaking changes to API
Addresses GitHub issue encode#9741
browniebroke
force-pushed
the
fix-mutable-default-arguments
branch
from
0b5cbea to
d2c9396
Compare
| extend-ignore = E501,W503,W504,B | ||
| extend-select = B006 |
There was a problem hiding this comment.
Disabled all flake8-bugbear checks except the mutable default arguments for now. We have a number of existing violations, but they feel out of scope of this PR.
5 participants
Note: Before submitting a code change, please review our contributing guidelines.
Description
Why this fix is important:
What was changed:
Testing results:
Fix #9741